Hi @ssardina, thanks for your PR! I see the black style check is still failing. Could you please fix it before this can be approved?

All the rest looks good.

Codecov Report

Attention: Patch coverage is 97.72727% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.37%. Comparing base (4ee8d63) to head (553de64).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   88.35%   88.37%   +0.02%     
==========================================
  Files          25       25              
  Lines        1794     1798       +4     
  Branches      333      333              
==========================================
+ Hits         1585     1589       +4     
  Misses        149      149              
  Partials       60       60              

No problem, I will check this and fix it. I need to understand what is happening, which is good, but it won't be done right away, but I am aware of it and will handle it. Thanks @francescofuggitti !

Sure, no worries. From the error logs, I think it's just a matter of invoking black on the project.

Great, thanks @ssardina!

@francescofuggitti I think I have fixed the formatting issues in both files. Note that because I have moved code in domain_to_string in formatter.py to core.py, some functions in formatter.py are not "private" anymore, because they are called from core.py so I removed the leading _ in their names, OK? For example:

So, formatter.py now provides a set of tools for formatting 😉

The formatting issue, via black, should be fixed. I think the coverage was not impacted and even seems to have gone up a bit (there seems to be some partial coverage but it was there before). Let me know if I am getting this wrong.

Hi @ssardina! Thank you for the update. I've approved the pipelines but it seems that the black check is still failing on the core.py module. Is there any issue with it?

Regarding the "private" to "non-private" functions, it's totally fine.

hi @francescofuggitti .. mmm you are right:

would reformat /home/runner/work/pddl/pddl/pddl/core.py

I found out that my black formatter was a bit newer (version 24) compared with the one in the repo (version 23) and incredibly they do different things!

Basically, with my black I get that core.py is fine, but parser/problem.py is not!

❯ black --version
black, 24.3.0 (compiled: yes)

❯ black pddl --check
would reformat /home/ssardina/git/planning/parsers/pddl.git/pddl/parser/problem.py

Oh no! 💥 💔 💥
1 file would be reformatted, 24 files would be left unchanged.

The change requested for problem.py would be:

❯ black pddl/parser/problem.py --check --diff
--- pddl/parser/problem.py	2024-03-16 04:30:27.755559+00:00
+++ pddl/parser/problem.py	2024-03-18 22:18:51.324753+00:00
@@ -180,13 +180,15 @@
             obj2 = self._objects_by_name.get(args[2])
             return EqualTo(obj1, obj2)
         else:
             name = args[1]
             terms = [
-                Constant(str(_term_name))
-                if self._objects_by_name.get(str(_term_name)) is None
-                else self._objects_by_name.get(str(_term_name))
+                (
+                    Constant(str(_term_name))
+                    if self._objects_by_name.get(str(_term_name)) is None
+                    else self._objects_by_name.get(str(_term_name))
+                )
                 for _term_name in args[2:-1]
             ]
             return Predicate(name, *terms)
 
     def f_exp(self, args):
would reformat pddl/parser/problem.py

Oh no! 💥 💔 💥
1 file would be reformatted.

The repo is using version 23 it seems:

black-check: black==23.3.0,click==8.1.7,mypy-extensions==1.0.0,packaging==24.0,pathspec==0.12.1,pip==24.0,platformdirs==4.2.0,setuptools==69.1.0,wheel==0.42.0

So I downgraded to 23.3.0 and I do get the same as the repo checker:

❯ black --version
black, 23.3.0 (compiled: yes)
Python (CPython) 3.10.12
❯ black pddl --check --diff
--- /home/ssardina/git/planning/parsers/pddl.git/pddl/core.py	2024-03-18 21:22:55.027221 +0000
+++ /home/ssardina/git/planning/parsers/pddl.git/pddl/core.py	2024-03-18 22:24:14.268847 +0000
@@ -218,13 +218,13 @@
         self._domain: Optional[Domain]
         self._domain_name: name_type
         self._domain, self._domain_name = self._parse_domain_and_domain_name(
             domain, domain_name
         )
-        self._requirements: Optional[AbstractSet[Requirements]] = (
-            self._parse_requirements(domain, requirements)
-        )
+        self._requirements: Optional[
+            AbstractSet[Requirements]
+        ] = self._parse_requirements(domain, requirements)
         self._objects: AbstractSet[Constant] = ensure_set(objects)
         self._init: AbstractSet[Formula] = ensure_set(init)
         self._goal: Formula = ensure(goal, And())
         self._metric: Optional[Metric] = metric
         validate(
would reformat /home/ssardina/git/planning/parsers/pddl.git/pddl/core.py

Oh no! 💥 💔 💥
1 file would be reformatted, 24 files would be left unchanged.

OK so a bit surprising but... I will do the changes under 23.3 black version and push the changes.

OK @francescofuggitti , let's see now, I have applied black 23.3 version to address the issue. I cannot see the check running again automatically, I guess you need to run it manually?

Also, I am a bit confused how one should work with this. The vscode black extension already uses version 24 (which, as seen here, would format files differently from version 23):

This means that I would have to run the black formatter manually outside vscode, which is OK but maybe it would be good to clarify this in the README and be explicit on what version of black needs to be used to pass the checks? Or am I missing something?

A wild way for things to break! Not sure what the long-term solution is, but for now we could probably just use a more recent version: https://github.com/AI-Planning/pddl/blob/main/Pipfile#L16

@francescofuggitti @marcofavorito ?

Yeah, this is a kind of general issue when working with multiple code style checkers (sometimes they do conflict with each other...).
In fact, now isort is not passing...

For this PR, we can try to apply some workaround (ignore some checks, etc). However, for the long-term solution, my proposal is to have the following:

• move all checks to ruff, to avoid issues across style checkers and to manage only one tool
• use pre-commit hooks to ensure that all checks are done at commit time
• provide a more comprehensive CONTRIBUTING.md with a step-by-step guide to set up the dev environment

What do you think?

As a pre-commit hook, it’ll start complaining and prevent the commit? Prevent the push? As long as the feedback is clear (I assume ruff would be), then that should make things easier, for sure. Having the block be on the PR review is a cumbersome process…

As a pre-commit hook, it’ll start complaining and prevent the commit? Prevent the push? As long as the feedback is clear (I assume ruff would be), then that should make things easier, for sure. Having the block be on the PR review is a cumbersome process…

Yes, the pre-commit will directly prevent the commit...

As of now, the best thing to do would be to install dev dependencies with pipenv install --dev and manually run tox before committing/pushing.

@ssardina, I've taken a closer look at the code, and I guess that to fix this PR you can do the following:

• remove from textwrap import indent from the formatter.py and add it to the core.py
• in the formatter.py, add a docstring to methods in lines 29 and 70
• run isort to sort imports on the core.py module
• run tox to check that everything is ok

Hi @francescofuggitti , thanks for that summary of what to do.. wow this tox is, or looks, amazing, can I use it with my course assessment and papers? 😆

OK I am close but yet there:

I know why black failed, still have version 24. easy...

need to investigate what is that flake8 and evaluation failed parts though..

Hi @francescofuggitti , thanks for that summary of what to do.. wow this tox is, or looks, amazing, can I use it with my course assessment and papers? 😆

OK I am close but yet there:

I know why black failed, still have version 24. easy...

need to investigate what is that flake8 and evaluation failed parts though..

Yeah, tox is really cool :)

Usually, when you're debugging tox errors, you may want to run only single pieces like tox -e flake8 or tox -e black-check. The failed evaluation should be fixed once black and flake are ok too. All commands that tox runs are in the tox.ini file in the root folder of the project.

Anyway, I think we're close to success here. To fix flake8 I guess the method def print_predicates_with_types(predicates: Collection): (line 83) is missing the docstring. And for black, as you correctly guessed, it's just a version issue.

Thank you again for putting your effort here, we're really pleased to get contributions from the community 🚀

Something is moving behind the scenes... #111 👀

Thanks @francescofuggitti for all the explanations, very useful. 🙏 and no worries on my side, I am actually learning quite a bit! 👍

I can now run tox targetted and I understand the issues it reports; there were 3 issues reported, very nice tools these ones.

Is it correct that you need to start the GH checks yourself manually right? I cannot see them triggering when I add to the PR

Ya, repo maintainers need to approve — it’s to prevent DDoS’ing a repo’s computation with a crazy # of PR’s.

green is nice 💚

Thanks @francescofuggitti & @haz for your patience.. 👍